-
Notifications
You must be signed in to change notification settings - Fork 276
Split MPP by maximizing expected delivered amount #2792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
renepickhardt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK (can't give a proper ACK in github on this repo)
As noted in my comment on the code I checked that finding the max should be equivalent to 1/x - 1/(c_1 - x) - ... - 1/(c_n - x). With my poor scala skills I belive the code computes that formular. (Actually https://www.codeconvert.ai/scala-to-python-converter suggested to me that my understanding of the scala code for private def optimizeExpectedValue seemed correctly in which case I think your code does what it is supposed to do.
That being said: I find it extremely elegant to do the binary search in this way to find the maximum of the expected value curve. Kudos to this! I made this less elegant in my eval framework as my code (with evaluating on a grid of 100 points) works with any probabilistic prior and I wanted to check also non uniform priors.
If I understand correctly you also didn't include your local knowledge / belief about remote channel's liquidity into the max expected value computation but only used the already allocated inflight htlcs. (I think substracting the used capacity is correctly)
ps: I am not sure if you get notifications on closed PR's but I commented with two diagrams that splitting of the liquidity which one has certainty about still makes sense.
All that being said: As mentioned on my original PR I would be very delighted and happy if you can share results from the mainnet A/B test of your node and give me permission to utilize them in the publication / paper that I expect to finalize soon. In particular I am curious if the method works as intendet.
Thank you for this patch!
eclair-core/src/main/scala/fr/acinq/eclair/router/RouteCalculation.scala
Outdated
Show resolved
Hide resolved
|
That PR was unfortunately forgotten as we had a lot on our plate around that time, do you think it makes sense to work on it now and integrate it before the next release @thomash-acinq? I'll spend time reviewing it if you think it's useful to have. |
As suggested by @renepickhardt in #2785
c059503 to
d2d43a3
Compare
eclair-core/src/main/scala/fr/acinq/eclair/remote/EclairInternalsSerializer.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/router/RouteCalculation.scala
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/router/RouteCalculation.scala
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/router/RouteCalculation.scala
Outdated
Show resolved
Hide resolved
it's still allowed for local channels (in several HTLCs)
eclair-core/src/main/scala/fr/acinq/eclair/router/RouteCalculation.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/router/RouteCalculation.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/router/RouteCalculation.scala
Show resolved
Hide resolved
|
@pm47 this will require a deployment of the front machines, and a change in the path-finding experiments to test this new splitting strategy. |
As suggested by @renepickhardt in #2785